-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change Elyra public API endpoint to ensure a correct URL for redirect #1461
Change Elyra public API endpoint to ensure a correct URL for redirect #1461
Conversation
Interesting -- not sure this solves the problem -- it'll just shift it to another problem. Although I agree, the internal routing structure of Dashboard should be maintained on our end... but we should probably change that property to be something else or have two properties or something. You're going to not be able to handle upgrades this way, since that route will just fail on the Dashboard as-is on upgrade. @harshad16 we should be careful about how things are handled here... can we use two params? One for "this is where runs exists" and one for "this is the path to the run, insert ID at the end" so that it always works and doesn't break. It'll still suck for the old notebooks with an out of date secret, but at least it won't go from working badly to a 404 page. EDIT: Also we need to handle upgrades / updating the secret when it lacks the suffix value in question. |
@andrewballantyne , i m not sure, which another problem will this cause?
Is this comment, in regards to dashboard-and-components would be agnostic each other in future.
Can please share some more information here,
Agreed, in long run we should focus on this. |
Hi @harshad16 can you share the testing details here please? I would like to replicate it to review this. |
Testing procedure:
|
thanks! ill try to review it today, if not ill get to that next Tuesday (I'm on to the rest of the week) |
@lucferbux @andrewballantyne please try to review in this week |
Will discuss with Harshad later today... holding until we cover my concerns with this PR. Apologizes for the delay, behind on emails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you do these changes (patch to apply below):
diff --git a/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts b/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
index c703e519..317fec25 100644
--- a/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
+++ b/frontend/src/concepts/pipelines/context/useManageElyraSecret.ts
@@ -4,7 +4,11 @@ import { createSecret, replaceSecret } from '~/api';
import { DSPipelineKind } from '~/k8sTypes';
import { generateElyraSecret } from '~/concepts/pipelines/elyra/utils';
import useAWSSecret from '~/concepts/secrets/apiHooks/useAWSSecret';
-import { ELYRA_SECRET_DATA_KEY, ELYRA_SECRET_DATA_TYPE } from '~/concepts/pipelines/elyra/const';
+import {
+ ELYRA_SECRET_DATA_ENDPOINT,
+ ELYRA_SECRET_DATA_KEY,
+ ELYRA_SECRET_DATA_TYPE,
+} from '~/concepts/pipelines/elyra/const';
const useManageElyraSecret = (
namespace: string,
@@ -41,9 +45,14 @@ const useManageElyraSecret = (
return;
}
try {
- const secretValue = JSON.parse(atob(elyraSecret.data?.[ELYRA_SECRET_DATA_KEY] || '{}'));
- if (secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS') {
- // Secret is invalid -- update it
+ const secretValue = JSON.parse(
+ atob(elyraSecret.data?.[ELYRA_SECRET_DATA_KEY] || '{ metadata: {} }'),
+ );
+ const usingOldDataType =
+ secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS';
+ const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/');
+ if (usingOldDataType || usingOldUrl) {
+ // Secret is out of date, update it
replaceSecret(
generateElyraSecret(
dataConnection.data,
diff --git a/frontend/src/concepts/pipelines/elyra/const.ts b/frontend/src/concepts/pipelines/elyra/const.ts
index 24c250aa..7db53078 100644
--- a/frontend/src/concepts/pipelines/elyra/const.ts
+++ b/frontend/src/concepts/pipelines/elyra/const.ts
@@ -3,4 +3,5 @@ import { PIPELINE_DEFINITION_NAME } from '~/concepts/pipelines/const';
export const ELYRA_SECRET_NAME = 'ds-pipeline-config';
export const ELYRA_SECRET_DATA_KEY = 'odh_dsp.json';
export const ELYRA_SECRET_DATA_TYPE = 'cos_auth_type';
+export const ELYRA_SECRET_DATA_ENDPOINT = 'public_api_endpoint';
export const ELYRA_ROLE_NAME = `ds-pipeline-user-access-${PIPELINE_DEFINITION_NAME}`;
diff --git a/frontend/src/concepts/pipelines/elyra/utils.ts b/frontend/src/concepts/pipelines/elyra/utils.ts
index 2888c525..66f761dc 100644
--- a/frontend/src/concepts/pipelines/elyra/utils.ts
+++ b/frontend/src/concepts/pipelines/elyra/utils.ts
@@ -2,6 +2,7 @@ import { Patch } from '@openshift/dynamic-plugin-sdk-utils';
import { AWSSecretKind, KnownLabels, NotebookKind, RoleBindingKind, SecretKind } from '~/k8sTypes';
import {
ELYRA_ROLE_NAME,
+ ELYRA_SECRET_DATA_ENDPOINT,
ELYRA_SECRET_DATA_KEY,
ELYRA_SECRET_DATA_TYPE,
ELYRA_SECRET_NAME,
@@ -63,7 +64,8 @@ export const generateElyraSecret = (
engine: 'Tekton',
auth_type: 'KUBERNETES_SERVICE_ACCOUNT_TOKEN',
api_endpoint: route,
- public_api_endpoint: `${location.origin}/pipelineRuns/${namespace}`,
+ // Append the id on the end to navigate to the details page for that PipelineRun
+ [ELYRA_SECRET_DATA_ENDPOINT]: `${location.origin}/pipelineRuns/${namespace}/pipelineRun/view/`,
[ELYRA_SECRET_DATA_TYPE]: 'KUBERNETES_SECRET',
cos_secret: dataConnectionName,
cos_endpoint: atob(dataConnectionData[AWS_KEYS.S3_ENDPOINT]),
That'll allow us to update the secret for existing items.
efbaa95
to
becccf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good... @harshad16 have you retested this to make sure it works?
); | ||
const usingOldDataType = | ||
secretValue.metadata[ELYRA_SECRET_DATA_TYPE] === 'USER_CREDENTIALS'; | ||
const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good... have you retested this to make sure it works?
I have tested the 2 scenario ,
- in a new cluster setup, it works as we want.
- In a old cluster setup, the value doesn't get updated.
i feel the reason is because are not checking the right condition:
const usingOldUrl = secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/'); | |
const usingOldUrl = !secretValue.metadata[ELYRA_SECRET_DATA_ENDPOINT].endsWith('/view/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooooh, good catch. That was supposed to be a not. @mlassak please make that change.
becccf7
to
0ac1dbf
Compare
Thanks for the updated. Pasting only the secret, the manage field show
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Resolves:opendatahub-io/notebooks#130
Complementary PR from notebooks: opendatahub-io/notebooks#137
Description
Change the Elyra public API endpoint URL so that it correctly references to the desired data science pipeline run details.
How Has This Been Tested?
Not fully tested yet, testing in progress
Test Impact
Request review criteria: